Skip to content

Conversation

@nagisa
Copy link
Owner

@nagisa nagisa commented Nov 5, 2025

cc @AlexanderSchuetz97 double-check that this works for you.

I ended up nuking a number of AsFilename and AsSymbolName impls still and made the remaining ones terser. We can easily add them back later on in a backwards-compatible manner if it turns out that the current impls don't serve some specific need for some reason or another.

@nagisa nagisa changed the title Nagisa/nostd fixups Prepare for 0.9.0 release Nov 5, 2025
@nagisa nagisa force-pushed the nagisa/nostd-fixups branch 2 times, most recently from 0d0f6ba to 6f25c6f Compare November 5, 2025 14:57
@AlexanderSchuetz97
Copy link
Contributor

My no-std use case only requires &str to work. This is fine for me.
I will "experimentally" use this branch in the project that needs it tomorrow. Up to you if you want to wait for that.
At first glance this looks good to me.

Copy link
Contributor

@AlexanderSchuetz97 AlexanderSchuetz97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are the only things that cought my eye
Also I have to say I dont understand your check_null_bytes fn in util, like it is very hard to read what it does. I just assume that it works as intended.

@AlexanderSchuetz97
Copy link
Contributor

AlexanderSchuetz97 commented Nov 5, 2025

Could you consider to do the seals the way I did them?
RustRover, a IDE which many people use for rust, isnt smart enough to realize that the functions are sealed.
Since we implement them for a common type string and friends, it will recommend them, this may be annoying to some people.
image

The way I sealed them works for rust rover.
Again this is not a problem for me personally. Its just, I know people that would nitpick this.

@nagisa
Copy link
Owner Author

nagisa commented Nov 5, 2025

Unfortunately the supertrait sealing makes the implementers not visible in the documentation, which I think is crucial to have here. We can explore other options, although at the same time: are these suggestions showing up even when the trait isn't imported? There should rarely be a need for anybody to import the trait.

@nagisa
Copy link
Owner Author

nagisa commented Nov 5, 2025

Hm, maybe we could still use super trait and have the methods on it, but instead of having a generic impl<T: SealedTrait> Trait for T, we'd have implementations of Trait for each individual type too... Let me try.

@AlexanderSchuetz97
Copy link
Contributor

AlexanderSchuetz97 commented Nov 5, 2025

The suggestion shows up when the trait is not imported. Its enough to have libloading in Cargo.toml.
It doesn't even need to be used at all.

Its not that big of a deal, just can be confusing.
Also, why does this showing up in the documentation matter?
The user of libloading should never call these functions, they are something internal to the crate.

I happen to coincidentally maintain an unrelated crate that requires usage of String and friends in CESU-8 byte encoding.
Sometimes CESU8 and Utf-8 overlap so I only need to allocate a buffer "sometimes".
There I chose a similar trait+closure approch and found it quite nice that my internal method is hidden in the doc.
At least my approach is, if the user is not supposed to call it then he shouldnt need to know about it.

@nagisa
Copy link
Owner Author

nagisa commented Nov 5, 2025

Also, why does this showing up in the documentation matter?
The user of libloading should never call these functions, they are something internal to the crate.

That's how you learn about which types you can use to pass in into Library::new or Library::get. One could argue that listing them manually somewhere would be good enough, but nothing really beats being able to click on AsSymbol and finding the following:

image

The internal method does not show up (in fact I especially hid it with doc(hidden) in the sealed method approach.) That said my idea of adjusting supertrait method does also work for doc generation so I'm happy to switch back to supertrait method.

@nagisa nagisa force-pushed the nagisa/nostd-fixups branch from b009346 to 14638bb Compare November 5, 2025 22:19
@AlexanderSchuetz97
Copy link
Contributor

Ah rustdoc must have not been smart enough to see my generic impl for T, that makes sense. Yes them showing up in the doc like that is indeed more important than not potentically annoying RustRover users (like me).

Good thinking with the manual impl, glad we could get that sorted out. I am still working on unrelated things so I can actually compile my inteded library with no-std and libloading as I intend to. I will report back once I have succeeded (or failed, but that is unlikely).

@nagisa nagisa force-pushed the nagisa/nostd-fixups branch from 14638bb to 44eb45b Compare November 5, 2025 22:21
@AlexanderSchuetz97
Copy link
Contributor

Alright all is working as expected for my use case. RustRover is also quiet now.
You can go ahead and merge this.

Was once again a pleasure working on this with you and I also learned something new!
Thank you very much.

* `impl AsSymbolName for &[u16]` hides unconditional allocation (user
  could pass a `&str` or `&[u8]` or `&CStr` instead all of which should
  be more straightforward.
* `impl AsFilename for &[T]` have awkward platform implications. With
  `T = u16` we require that buffer is valid UTF-16, for `T = u8` we
  require that it is UTF-8 but only on Windows. I'm not super
  comfortable with having these types in the cross-platform API
  surface currently. We can explore adding something as a follow-up, but
  for now no-std users will have to live with passing in a `&str`.
* Similarly for `impl AsFilename for CString` family.
* There are a couple of no-std capable tests that sufficiently test the
  functionality of no-std behaviour. For the rest it is fine to require
  that libstd is available. So this removes most of the duplicate no-std
  tests.
@nagisa nagisa force-pushed the nagisa/nostd-fixups branch from 44eb45b to e1416f7 Compare November 5, 2025 22:56
@nagisa nagisa closed this Nov 5, 2025
@nagisa nagisa deleted the nagisa/nostd-fixups branch November 5, 2025 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants